feat(logpoller): use shared framework metrics where applicable#632
feat(logpoller): use shared framework metrics where applicable#632jadepark-dev wants to merge 3 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates TON logpoller observability to use the shared chainlink-framework logpoller metrics where applicable, replacing several TON-specific metrics.
Changes:
- Replace observed store query/insert metrics with
frameworkmetrics.GenericLogPollerMetricscalls (durations, dataset size, logs inserted). - Add a “log discovery latency” metric derived from
Log.TxTimestampduringSaveLogs. - Remove several TON-specific Prometheus/OTel metrics and their helper methods (e.g., query duration/result size, logs/blocks processed).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/logpoller/service.go | Stops emitting the “blocks processed” metric during runs. |
| pkg/logpoller/observed_log_store.go | Switches to framework metrics and adds log discovery latency instrumentation. |
| pkg/logpoller/observed_filter_store.go | Switches query duration/result-size instrumentation to framework metrics. |
| pkg/logpoller/metrics.go | Removes several TON-specific metrics and initializes shared framework logpoller metrics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| newestTimestamp = l.TxTimestamp | ||
| } | ||
| } | ||
| o.metrics.frameworkMetrics.RecordLogDiscoveryLatency(ctx, time.Since(newestTimestamp).Seconds()) |
There was a problem hiding this comment.
time.Since(newestTimestamp) can return a negative duration if TxTimestamp is ahead of the local wall clock (clock skew or differing time sources), which would record a negative discovery latency. Consider clamping the value to >= 0 before emitting the metric to avoid invalid/confusing latency datapoints.
| o.metrics.frameworkMetrics.RecordLogDiscoveryLatency(ctx, time.Since(newestTimestamp).Seconds()) | |
| latency := time.Since(newestTimestamp) | |
| if latency < 0 { | |
| latency = 0 | |
| } | |
| o.metrics.frameworkMetrics.RecordLogDiscoveryLatency(ctx, latency.Seconds()) |
| lp.lastProcessedBlockSeqNo = blockRange.ToSeqNo() | ||
| lp.metrics.SetLastProcessedBlock(ctx, lp.lastProcessedBlockSeqNo) | ||
| lp.metrics.AddBlocksProcessed(ctx, int64(blockRange.ToSeqNo()-blockRange.FromSeqNo())) | ||
|
|
There was a problem hiding this comment.
AddBlocksProcessed was removed and there is no replacement call to a shared/framework metric here, so the logpoller will no longer emit any “blocks processed” metric for a successful run. If blocks-processed is still a required SLI, record it via the new frameworkMetrics (or keep the existing metric) rather than dropping it silently; otherwise please update the PR scope/description to clarify the intentional removal.
| // Query metrics for observed stores | ||
| promTonLpQueryDuration = promauto.NewGaugeVec(prometheus.GaugeOpts{ | ||
| Name: "ton_logpoller_query_duration_seconds", | ||
| Help: "Duration of last database query by operation", | ||
| }, []string{"chainID", "query", "type"}) | ||
|
|
||
| promTonLpAddressesMonitored = promauto.NewGaugeVec(prometheus.GaugeOpts{ | ||
| Name: "ton_logpoller_addresses_monitored", | ||
| Help: "Number of addresses being monitored", | ||
| }, []string{"chainID"}) |
There was a problem hiding this comment.
This change removes several Prometheus/OTel query and ingestion metrics (e.g. ton_logpoller_query_duration_seconds, ton_logpoller_query_result_size, and the logs/blocks processed counters) from this package. That is an operationally breaking change for any existing dashboards/alerts scraping the ton_logpoller_* series; consider keeping backward-compatible aliases during a deprecation window or documenting the metric renames/removals as part of the rollout.
No description provided.